rc14 #209
rc14 #209
Conversation
* Export signature method * Modify calling signTypedData by wallet client * Upload pnpm-lock.yaml * Export some types * Refactor nftClient in order to better generate react sdk * Fix test about nftClient * Fix integration test * Rename SignatureHelpParameter to PermissionSignatureRequest
edisonz0718
requested review from
DonFungible,
jacob-tucker and
AndyBoWu
as code owners
WalkthroughThe updates to the core SDK involve version upgrades and substantial enhancements across several components. Key changes include new exports, type modifications, and method signature updates. The integration of Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
packages/core-sdk/src/types/common.ts (1)
10-18: The expandedPermissionSignatureRequesttype definition is comprehensive and aligns with the SDK's enhanced functionality. Consider adding comments to each field for better clarity and maintainability.packages/core-sdk/src/resources/nftClient.ts (1)
Line range hint
35-65: The updatedcreateNFTCollectionmethod inNftClientuses direct type references and includes comprehensive validation logic. Consider enhancing the error messages to provide more specific debugging information.packages/core-sdk/src/index.ts (1)
Line range hint
9-96: The new exports and type definitions inindex.tsenhance the SDK's functionality and modularity. Consider adding documentation for these new exports to improve the developer experience.packages/core-sdk/src/utils/sign.ts (1)
Line range hint
20-52: The error handling is robust, ensuring the wallet client's capabilities are checked. Consider adding more specific error messages for better user guidance.- throw new Error("The wallet client does not support signTypedData, please try again."); + throw new Error("The wallet client does not support signTypedData. Ensure your wallet client is updated or supports this feature."); - throw new Error("The wallet client does not have an account, please try again."); + throw new Error("The wallet client does not have an account. Please check your wallet client configuration.");packages/core-sdk/test/unit/resources/ipAsset.test.ts (1)
Line range hint
75-98: Handle potential issues whenwalletlackssignTypedData.- const walletMock = createMock<WalletClient>(); - walletMock.account = createMock<Account>(); - ipAssetClient = new IPAssetClient(rpcMock, walletMock, "sepolia"); + // Ensure walletMock is properly initialized with signTypedData or handle the absence gracefully + const walletMock = createMock<WalletClient>({ signTypedData: sinon.stub().resolves("signature") }); + walletMock.account = createMock<Account>(); + ipAssetClient = new IPAssetClient(rpcMock, walletMock, "sepolia");This test case simulates a scenario where the
walletobject does not have thesignTypedDatamethod, which throws an error. It's good to simulate this scenario, but ensure that thewalletMockis initialized in a way that reflects realistic usage patterns, possibly by providing a default stub forsignTypedData.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (18)
- packages/core-sdk/package.json (1 hunks)
- packages/core-sdk/src/index.ts (4 hunks)
- packages/core-sdk/src/resources/ipAsset.ts (6 hunks)
- packages/core-sdk/src/resources/license.ts (4 hunks)
- packages/core-sdk/src/resources/nftClient.ts (2 hunks)
- packages/core-sdk/src/resources/permission.ts (3 hunks)
- packages/core-sdk/src/resources/royalty.ts (3 hunks)
- packages/core-sdk/src/types/common.ts (1 hunks)
- packages/core-sdk/src/types/resources/license.ts (3 hunks)
- packages/core-sdk/src/types/resources/nftClient.ts (1 hunks)
- packages/core-sdk/src/types/resources/royalty.ts (1 hunks)
- packages/core-sdk/src/utils/sign.ts (2 hunks)
- packages/core-sdk/test/integration/ipAccount.test.ts (3 hunks)
- packages/core-sdk/test/integration/ipAsset.test.ts (1 hunks)
- packages/core-sdk/test/unit/resources/ipAsset.test.ts (3 hunks)
- packages/core-sdk/test/unit/resources/nftClient.test.ts (1 hunks)
- packages/core-sdk/test/unit/resources/permission.test.ts (3 hunks)
- packages/core-sdk/test/unit/utils/sign.test.ts (1 hunks)
Files skipped from review due to trivial changes (3)
- packages/core-sdk/package.json
- packages/core-sdk/src/types/resources/nftClient.ts
- packages/core-sdk/src/types/resources/royalty.ts
Additional context used
Biome
packages/core-sdk/test/unit/resources/permission.test.ts
[error] 15-30: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
Additional comments not posted (19)
packages/core-sdk/src/types/common.ts (1)
1-1: The import statement correctly includesAddressandWalletClientfrom "viem", aligning with the updated type usage across the SDK.packages/core-sdk/src/types/resources/license.ts (2)
52-52: The updatedmintingFeefield inRegisterCommercialRemixPILRequestto acceptstring | number | bigintenhances flexibility and aligns with the SDK's broader type support.
65-68: The newAttachLicenseTermsResponsetype, includingtxHashand an optionalsuccessfield, is well-defined and useful for tracking the outcomes of license term attachments.packages/core-sdk/test/integration/ipAccount.test.ts (3)
3-10: LGTM! The new imports are necessary for the updated tests.
Line range hint
70-81: LGTM! The update to usewalletClientaligns with the SDK changes.
Line range hint
70-81: The test case logic is consistent and correctly implements the new SDK functionality.packages/core-sdk/src/utils/sign.ts (1)
1-19: LGTM! The updated imports and the detailed function documentation enhance clarity and maintainability.packages/core-sdk/test/unit/utils/sign.test.ts (2)
3-7: LGTM! The new imports are correctly used for the updated tests.
11-40: The updates to the test cases are thorough and effectively simulate the new error handling scenarios in thegetPermissionSignaturefunction.packages/core-sdk/test/unit/resources/nftClient.test.ts (1)
105-106: The updates to the test cases reflect the simplified method signatures in theNftClientclass and are correctly implemented.packages/core-sdk/test/integration/ipAsset.test.ts (1)
141-141: EnsurenftContractis not null before assignment.Consider adding a comment explaining why this check is necessary, to improve code readability.
packages/core-sdk/src/resources/royalty.ts (1)
257-257: Improve error handling ingetRoyaltyVaultAddress.Consider providing more detailed error messages to help with debugging.
packages/core-sdk/src/resources/permission.ts (1)
132-132: Ensurethis.walletis always of typeWalletClient.Verify that
this.walletis always correctly instantiated asWalletClientto avoid runtime type errors.packages/core-sdk/test/unit/resources/permission.test.ts (1)
20-22: Improve testing ofsignTypedDatamethod.Consider adding more comprehensive tests to ensure that the
signTypedDatamethod is correctly handled in all scenarios.Also applies to: 191-206
packages/core-sdk/src/resources/license.ts (3)
25-26: LGTM! The addition of new typesAttachLicenseTermsResponseandLicenseTermsIdaligns with the PR's objective to update type definitions.
Line range hint
162-200: The updated method signature forattachLicenseTermsnow correctly returnsAttachLicenseTermsResponse. Good use of detailed error handling and conditional logic to ensure robustness.
296-296: The updated method signature forgetLicenseTermsnow correctly returnsPiLicenseTemplateGetLicenseTermsResponse. This change ensures type consistency across the application.packages/core-sdk/src/resources/ipAsset.ts (1)
1-1: LGTM! The updated imports includeWalletClient, which is used to replaceaccountwithwalletthroughout the class, aligning with the PR's objectives.packages/core-sdk/test/unit/resources/ipAsset.test.ts (1)
21-22: Ensure thesignTypedDatamethod is properly mocked.This change properly mocks the
signTypedDatamethod ofwalletMockto return a fixed hash. This is crucial for consistent test results.
Summary by CodeRabbit
New Features
IPAccountClientandRoyaltyClientto the SDK.AttachLicenseTermsResponse,LicenseTermsId,ClaimRevenueRequest,ClaimRevenueResponse, andPermissionSignatureRequest.getPermissionSignaturefunction for permission management.Improvements
attachLicenseTermsandgetLicenseTermsmethods for enhanced license management.createNFTCollectionmethod inNftClient.RoyaltyVaultAddresstype withAddressin royalty management.Refactor
accountwithwalletof typeWalletClientacross various modules.getPermissionSignaturefunction for better code clarity and performance.Bug Fixes
nftContractis not null before assignment in integration tests.Tests